Expose scrollTo and scrollToItem on imperative ref by weltenwort · Pull Request #6042 · elastic/eui

weltenwort · 2022-07-11T15:59:33Z

📝 Summary

This exposes the scrollTo() and scrollToItem() methods on the imperative API of the grid component. It's a break-out PR from #6028 (#6024).

Checklist

cee-chen · 2022-07-11T16:25:19Z

@weltenwort this looks great to me! Any objections if I push up Cypress tests to your branch for these 2 new APIs?

weltenwort · 2022-07-11T16:26:41Z

I accidentally submitted this too early and have since reverted it to a draft because I wanted to add tests. 🙈 Any help with those would be appreciated.

kibanamachine · 2022-07-11T16:31:48Z

Preview documentation changes for this PR: https://eui.elastic.co/pr_6042/

cee-chen · 2022-07-11T16:42:38Z

Yes, for sure! You'll want ref.spec.tsx and to add a new describe block for each API. The grid in this example is already virtualized and has lots of room for scrolling, which is nice, because we won't have to set that up manually. You should be able to do something like:

  describe('scrollToItem', () => {
    it('scrolls to a specific cell position, rendering the cell', () => {
      cy.get('[data-gridcell-row-index="90"]']).should('not.exist');
      ref.current.scrollToItem({ rowIndex: 90, columnIndex: 5 });
      cy.get('[data-gridcell-row-index="90"]']).should('exist');
    });
  });

  describe('scrollTo', () => {
    it('scrolls the grid to a specified position', () => {
      cy.get('.euiDataGrid__virtualized']).its('scrollY').should('equal', 0);
      ref.current.scrollTo({ scrollTop: 500, scrollLeft: 0 });
      cy.get('.euiDataGrid__virtualized']).its('scrollY').should('equal', 500);
    });
  });

I didn't actually run the above so it's possible I have some syntax incorrect - feel free to bug me if so!

Also, I can't remember if the Cypress docs mention it, but the CLI command you'll want to run/test this locally is yarn test-cypress-dev --skip-css

kibanamachine · 2022-07-11T17:08:38Z

Preview documentation changes for this PR: https://eui.elastic.co/pr_6042/

cee-chen · 2022-07-11T17:24:48Z

src-docs/src/views/datagrid/advanced/datagrid_advanced_example.js

+            <li>
+              <p>
+                <EuiCode>
+                  scrollTo({'{ scrollLeft: number, scrollTop: number }'})
+                </EuiCode>{' '}
+                - scrolls the grid to the specified horizontal and vertical
+                pixel offsets.
+              </p>
+            </li>
+            <li>
+              <p>
+                <EuiCode>
+                  scrollToItem(
+                  {
+                    '{align: string = "auto", columnIndex?: number, rowIndex?: number }'
+                  }
+                  )
+                </EuiCode>{' '}
+                - scrolls the grid to the specified row and columns indices
+              </p>
+            </li>


Couple quick documentation notes:

I think we should link to react-window's docs (https://react-window.vercel.app/#/api/FixedSizeGrid#methods) rather than write our own - in general since we're just passing their methods as-is, their docs will always be more up to date than ours.

I'm also tempted to nest another <ul> titled react-window methods (or similar) for this section - this is important because of how EUI's ref APIs handle rowIndex vs how react-window's APIs handle rowIndex. See the below callout:

EUI expects the rowIndex received from the original unsorted/unpaginated consumer data. react-window does not care at all about that and wants what EUI refers to as the visibleRowIndex (i.e., the row index after sorting and pagination). This is a confusing but important difference that could potentially bite consumers if not clarified, and I think it's worth separating out react-window's ref APIs vs EUI's ref APIs for that reason.

@chandlerprall any thoughts here?

After a bit of poking, I think this is how I'd prefer to separate the API documentation for clarity:

LMK what y'all think 🤔

I'd also love to update src-docs/src/views/datagrid/advanced/ref.tsx with a Scroll to cell button that uses scrollToItem, which I think will help make it a bit clearer to users the difference between the two APIs on a sorted grid!

Separating the new methods into their own list makes sense to me. I think it draws a good distinction between the logic EUI provides vs. these pass-through functions. That would also provide a natural place to link to react-window's docs

Ok, I'll split the react-window API into its own section. Any opinion which section should own the example? Should it belong to the "Ref methods" or "react-window methods" section?

I put it after the second section for now since it's pretty short. Let me know what you think.

kibanamachine · 2022-07-11T18:23:21Z

Preview documentation changes for this PR: https://eui.elastic.co/pr_6042/

src/components/datagrid/utils/ref.spec.tsx

upcoming_changelogs/6042.md

src/components/datagrid/utils/ref.ts

Co-authored-by: Constance <constancecchen@users.noreply.github.com>

kibanamachine · 2022-07-12T09:39:11Z

Preview documentation changes for this PR: https://eui.elastic.co/pr_6042/

kibanamachine · 2022-07-12T11:16:41Z

Preview documentation changes for this PR: https://eui.elastic.co/pr_6042/

cee-chen · 2022-07-12T19:52:03Z

Looks fantastic, thanks so much @weltenwort! I have a couple super minor docs comments/tweaks that I'll go ahead and push up to your branch since I'm a half-day timezone away from you (and very slow in the mornings 😅) and don't want to delay your PR another full day. Excited to see the next PR with RowHeightUtils test improvements, that looked really cool from what I saw in the original PR!

- combine sections and use EuiTitle tag for react-window methods - Fix link to react-window docs - minor grammar tweaks

kibanamachine · 2022-07-12T20:34:45Z

Preview documentation changes for this PR: https://eui.elastic.co/pr_6042/

weltenwort · 2022-07-13T10:12:09Z

@constancecchen thanks so much for helping me with the docs ❤️ and your cypress snippets got me started very quickly

Excited to see the next PR with RowHeightUtils test improvements

sure thing, ~~I'll try to submit it today~~ I submitted those in #6050

kibanamachine · 2022-07-21T06:22:57Z

Preview documentation changes for this PR: https://eui.elastic.co/pr_6042/

weltenwort added 2 commits July 11, 2022 15:52

Expose scrollTo and scrollToItem on imperative ref

29f9f12

Add documentation for scrollTo and scrollToItem

da364e5

weltenwort marked this pull request as draft July 11, 2022 15:59

cee-chen reviewed Jul 11, 2022

View reviewed changes

weltenwort added 2 commits July 11, 2022 17:50

Add tests for scrollTo and scrollToItem

f9894db

Add changelog entry

39f655e

weltenwort self-assigned this Jul 11, 2022

weltenwort added the data grid label Jul 11, 2022

weltenwort marked this pull request as ready for review July 11, 2022 18:13

cee-chen reviewed Jul 11, 2022

View reviewed changes

src/components/datagrid/utils/ref.spec.tsx Show resolved Hide resolved

cee-chen reviewed Jul 11, 2022

View reviewed changes

upcoming_changelogs/6042.md Outdated Show resolved Hide resolved

cee-chen reviewed Jul 11, 2022

View reviewed changes

src/components/datagrid/utils/ref.ts Show resolved Hide resolved

weltenwort and others added 2 commits July 12, 2022 11:05

Update upcoming_changelogs/6042.md

c56d705

Co-authored-by: Constance <constancecchen@users.noreply.github.com>

Merge branch 'main' into data-grid-scroll-to-api

88ab164

Move react-window API docs into separate section

530a33c

cee-chen added 2 commits July 12, 2022 12:52

Merge branch 'main' into data-grid-scroll-to-api

5254cb4

[PR feedback] Docs tweaks

80ecccf

- combine sections and use EuiTitle tag for react-window methods - Fix link to react-window docs - minor grammar tweaks

cee-chen approved these changes Jul 12, 2022

View reviewed changes

cee-chen enabled auto-merge (squash) July 12, 2022 20:14

cee-chen merged commit cb937f4 into elastic:main Jul 12, 2022

breehall mentioned this pull request Jul 14, 2022

Upgrade EUI to v60.3.0 elastic/kibana#136405

Merged

weltenwort mentioned this pull request Jul 19, 2022

[EuiDataGrid] Reduce layout shifts when auto-sizing overscan rows #6028

Closed

10 tasks

Conversation

weltenwort commented Jul 11, 2022 • edited by matschaffer Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

📝 Summary

Checklist

Uh oh!

cee-chen commented Jul 11, 2022

Uh oh!

weltenwort commented Jul 11, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kibanamachine commented Jul 11, 2022

Uh oh!

cee-chen commented Jul 11, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kibanamachine commented Jul 11, 2022

Uh oh!

cee-chen Jul 11, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cee-chen Jul 11, 2022

Choose a reason for hiding this comment

Uh oh!

cee-chen Jul 11, 2022

Choose a reason for hiding this comment

Uh oh!

chandlerprall Jul 11, 2022

Choose a reason for hiding this comment

Uh oh!

weltenwort Jul 12, 2022

Choose a reason for hiding this comment

Uh oh!

weltenwort Jul 12, 2022

Choose a reason for hiding this comment

Uh oh!

kibanamachine commented Jul 11, 2022

Uh oh!

Uh oh!

Uh oh!

Uh oh!

kibanamachine commented Jul 12, 2022

Uh oh!

kibanamachine commented Jul 12, 2022

Uh oh!

cee-chen commented Jul 12, 2022

Uh oh!

kibanamachine commented Jul 12, 2022

Uh oh!

weltenwort commented Jul 13, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kibanamachine commented Jul 21, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

weltenwort commented Jul 11, 2022 •

edited by matschaffer

Loading

weltenwort commented Jul 11, 2022 •

edited

Loading

cee-chen commented Jul 11, 2022 •

edited

Loading

cee-chen Jul 11, 2022 •

edited

Loading

weltenwort commented Jul 13, 2022 •

edited

Loading